Skip to content

Conversation

@Birkbjo
Copy link
Member

@Birkbjo Birkbjo commented Sep 15, 2022

Previously the --customContext option for the up-command was just a boolean, and it used the cluster name for the contextPath when this was set. I think it should be possible to set this independently of the cluster name, as it's very useful if you would want additional levels, eg. /dhis2/dev. This is somewhat related to CLI-75, since having a cluster with a path-delimeter (/), results in very buggy behavior.

I've tried to implement this in a non-breaking way. Eg. we still support using a flag -c, and we will then use the cluster name.

@Birkbjo Birkbjo force-pushed the feat/cluster-context-path branch 2 times, most recently from 3ae135a to 8776455 Compare September 15, 2022 23:44
@Birkbjo Birkbjo force-pushed the feat/cluster-context-path branch 2 times, most recently from d3d2f29 to 17c6ae1 Compare September 16, 2022 00:05
@Birkbjo Birkbjo force-pushed the feat/cluster-context-path branch from 17c6ae1 to cf461d7 Compare September 16, 2022 00:07
@Birkbjo Birkbjo assigned amcgee and ghost Sep 16, 2022
@Birkbjo Birkbjo changed the title feat(cluster): context path feat(cluster): customisable context path Sep 16, 2022
@Birkbjo Birkbjo unassigned amcgee and ghost Sep 16, 2022
@Birkbjo Birkbjo requested review from a user and amcgee September 16, 2022 10:32
Copy link
Member

@amcgee amcgee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't tested myself but this looks good, thanks @Birkbjo! Some minor comments, not blocking.

Comment on lines +39 to +41
const anyCustomContext = clusters.some(
cluster => cluster.contextPath !== ''
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just always show the context, even if none are custom?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could. The idea was that this is most likely not relevant to most users. I think very few are actually using this context-path, and it might be confusing if you don't know what it does?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could go either way here... I also think it would be confusing if d2 cluster list sometimes renders in a different format if you happen to have a custom context on one cluster you're running. It would make any kind of parsing of the output difficult as well, though we're not exactly well setup for that as it is

cluster.dhis2Version,
cluster.dbVersion,
formatStatus(status),
].concat(anyCustomContext ? cluster.contextPath : [])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe cluster.contextPath || '/' ?

Copy link
Member Author

@Birkbjo Birkbjo Sep 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you think we should show this regardless of any set custom-context, I agree, we should probably do that.

Or do you mean we should show '/' instead of an empty cell for non-custom contexts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean we should show / instead of an empty cell for non-custom contexts, since that's the path that they can use to access the instance

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll defer to Austin's review on this. I'm not super familiar with this bit of functionality as a user, changes look good to me 👍️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants